Skip to content

Conversation

@alphaprinz
Copy link
Contributor

@alphaprinz alphaprinz commented Apr 3, 2025

Describe the Problem

We want to allow user to describe an external Kafka server in a connection file, but allow to specify to specify Kafka topic per notification conf (instead of in the connection).

Explain the Changes

TopicConfigurations' TopicArn field can be of this syntax "kafka:::topic//", where:
-if connection is not specified, the default account's connection is used.
-Kafka topic in TopicArn takes precedence over Kafka topic in connection.
-backward compatible - you can still put just the connection in TopicArn, like now

Issues: Fixed #xxx / Gap #xxx

Testing Instructions:

  1. Set "default_connection" field in NC user.

  2. Create a connection file for kafka (with or without topic field).
    {
    "notification_protocol": "kafka",
    "name": "k",
    "topic": "mytopic",
    "kafka_options_object": "{"metadata.broker.list":"localhost:9092"}"
    }

  3. Configure a notification with override object:
    {
    "TopicConfigurations": [
    {
    "Id": "first",
    "TopicArn": "kafka:::topic//override",
    "Events": []
    }
    ]
    }

  4. Create an event (ie upload object to bucket with notification).

  5. The notification will be sent with user's default connection with topic "override".

  • Doc added/updated - TBD
  • Tests added

@alphaprinz alphaprinz requested a review from guymguym April 3, 2025 03:19
@alphaprinz alphaprinz force-pushed the notif_kafka_connect2 branch 2 times, most recently from 391fe76 to b32378a Compare April 3, 2025 05:39
Copy link
Member

@guymguym guymguym left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should follow the arn structure format, see comments posted above.

@alphaprinz alphaprinz force-pushed the notif_kafka_connect2 branch from b32378a to 5d5ee14 Compare April 10, 2025 18:08
@pull-request-size pull-request-size bot added size/S and removed size/M labels Apr 10, 2025
@alphaprinz alphaprinz requested a review from guymguym April 10, 2025 21:28
@alphaprinz alphaprinz force-pushed the notif_kafka_connect2 branch from 5d5ee14 to f1e6bd3 Compare May 6, 2025 21:00
@pull-request-size pull-request-size bot added size/M and removed size/S labels May 6, 2025
@alphaprinz alphaprinz force-pushed the notif_kafka_connect2 branch 4 times, most recently from 56759b8 to 3871f68 Compare May 9, 2025 17:49
@alphaprinz alphaprinz marked this pull request as ready for review May 9, 2025 18:12
@alphaprinz alphaprinz force-pushed the notif_kafka_connect2 branch from 3871f68 to 6447e8e Compare May 9, 2025 18:13
@alphaprinz alphaprinz requested a review from nadavMiz May 13, 2025 14:52
const VALID_OPTIONS_ACCOUNT = {
'add': new Set(['name', 'uid', 'gid', 'supplemental_groups', 'new_buckets_path', 'user', 'access_key', 'secret_key', 'fs_backend', 'allow_bucket_creation', 'force_md5_etag', 'iam_operate_on_root_account', FROM_FILE, ...CLI_MUTUAL_OPTIONS]),
'update': new Set(['name', 'uid', 'gid', 'supplemental_groups', 'new_buckets_path', 'user', 'access_key', 'secret_key', 'fs_backend', 'allow_bucket_creation', 'force_md5_etag', 'iam_operate_on_root_account', 'new_name', 'regenerate', ...CLI_MUTUAL_OPTIONS]),
'add': new Set(['name', 'uid', 'gid', 'supplemental_groups', 'new_buckets_path', 'user', 'access_key', 'secret_key', 'fs_backend', 'allow_bucket_creation', 'force_md5_etag', 'iam_operate_on_root_account', 'default_connection', FROM_FILE, ...CLI_MUTUAL_OPTIONS]),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this part of the account configuration. shouldn't it be per bucket?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently in Marc's project the user and connection creation would be done by one admin user (with manage_nsfs cli), while buckets and their notif conf would be done by other (not-admin) users (with s3 rest api). So the admin would be able to do a once-per-system users and connection creation (including user's default connection), and then the other users wouldn't need to know about connections at all.

@alphaprinz alphaprinz force-pushed the notif_kafka_connect2 branch from 6447e8e to 9d199ad Compare May 14, 2025 00:32
@alphaprinz alphaprinz requested a review from nadavMiz May 14, 2025 05:45
conf.event = conf.Event;
conf.topic = conf.Topic;
//handle Kafka's topic synax, if present
if (conf.Topic && conf.Topic.length > 0 && conf.Topic[0].startsWith('kafka:::topic/')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it be also good to have a validation on the cli level. that you wouldn't be able to configure default_connection of this format?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand.
Account's default_connection is just a string. It doesn't have a "special" syntax (as opposed to conf.Topic's optional "kafka:::topic" syntax).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, if it doesn't have a "special" syntax, its fine

Copy link
Contributor

@nadavMiz nadavMiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alphaprinz alphaprinz force-pushed the notif_kafka_connect2 branch 2 times, most recently from 7961043 to 399e208 Compare May 20, 2025 17:46
@nimrod-becker
Copy link
Contributor

Guy's comments were resolved, we can merge

@alphaprinz alphaprinz force-pushed the notif_kafka_connect2 branch from 399e208 to 3c9806d Compare May 22, 2025 15:53
@nimrod-becker nimrod-becker merged commit 0557aa1 into noobaa:master May 22, 2025
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants